Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore collapsers #701

Merged
merged 18 commits into from
Oct 10, 2023
Merged

Conversation

fcollonval
Copy link
Collaborator

Fixes #679

@fcollonval fcollonval force-pushed the fix/679-CM6-Restore-collapser branch from b45b11f to 8dd6bc6 Compare October 3, 2023 09:53
@fcollonval
Copy link
Collaborator Author

fcollonval commented Oct 3, 2023

bot please update playwright snapshots

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2023

Playwright windows-latest snapshots updated.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2023

Playwright ubuntu-22.04 snapshots updated.

@fcollonval
Copy link
Collaborator Author

Kicking the CI

@fcollonval fcollonval closed this Oct 3, 2023
@fcollonval fcollonval reopened this Oct 3, 2023
@fcollonval
Copy link
Collaborator Author

bot please update playwright snapshots

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Playwright windows-latest snapshots updated.

Copy link
Collaborator

@vidartf vidartf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked to closely at this yet, but a skim through showed some weird differences between linux and Windows UI test screenshots?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The windows version retains the lock spacer, while the linux version does not?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linux version has ... X unchanged lines ... while Windows version does not have ellipsis?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vidartf Indeed that's strange.

@krassowski
Copy link
Member

The issue with divergent snapshots is because one of the updates failed (the Linux one) because between starting the job and finishing it someone pushed another commit to the branch which prevented it from pushing the udpated screenshots https://github.com/jupyter/nbdime/actions/runs/6420290475/job/17432051180. Because the jobs run sequentially the other one did not fail because it already fetched the updated git repo. To prevent inconsistent state we could make the entire job fail if any of them fails. For now:

bot please update playwright snapshots

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2023

Playwright ubuntu-22.04 snapshots updated.

@HaudinFlorence
Copy link
Contributor

HaudinFlorence commented Oct 6, 2023

@krassowski Ok thanks for pointing this. This may be my fault then because I pushed a new commit on the branch. But now the three dots ellipsis does not appear neither on windows nor on linux snapshots. It appears that this change vanishes out. I will restablish it.

@HaudinFlorence
Copy link
Contributor

bot please update playwright snapshots

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2023

Playwright windows-latest snapshots updated.

@HaudinFlorence
Copy link
Contributor

HaudinFlorence commented Oct 6, 2023

Issue is fixed, the screenshots are now ok and tests are green.

toDOM(view: EditorView) {
let outer = document.createElement('div');
outer.className = 'cm-collapsedLines';
outer.textContent = view.state.phrase('...$ unchanged lines...', this.lines);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want this to look similar to older versions of nbdime, we could simply have this be (...). Maybe having (...) $ unchanged lines ? Mostly subjective taste at this point, so I'm happy to leave the final judgment to you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I proposed this mixed version between the previous (...) version and the new text-only one but I don't have a strong opinion on it. Maybe @fcollonval @krassowski have some.

@fcollonval
Copy link
Collaborator Author

bot please update playwright snapshots

@github-actions
Copy link
Contributor

Playwright windows-latest snapshots updated.

@github-actions
Copy link
Contributor

Playwright ubuntu-22.04 snapshots updated.

@fcollonval fcollonval closed this Oct 10, 2023
@fcollonval fcollonval reopened this Oct 10, 2023
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@fcollonval fcollonval merged commit 40e692a into jupyter:master Oct 10, 2023
13 checks passed
@fcollonval fcollonval deleted the fix/679-CM6-Restore-collapser branch October 10, 2023 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CM6: Restore collapser
4 participants